GemTalk / RemoteServiceReplication

MIT License
0 stars 4 forks source link

Ensure instances of RsrConnection cannot be committed #142

Open ericwinger opened 1 year ago

ericwinger commented 1 year ago

Every so often, not always, while replicating RSR services, I try to commit and get this error:

a TransactionError occurred (error 2407), The object Semaphore(396033793) may not be committed, 'cannot be committed, instances of its class are non-persistent'

After some finagling, I was able to list references and trace the semaphore back to the connection which every service holds onto (in memory because listReferences: and fastListReferences required aborts that would lose data)

Object _objectForOop: 396033793 "'Semaphore(396033793)'"
SystemRepository listReferencesInMemory: (Array with: (Object _objectForOop: 396033793))

Object _objectForOop: 395845121 "'aRsrThreadSafeNumericSpigot'"
SystemRepository listReferencesInMemory: (Array with: (Object _objectForOop: 395845121))

Object _objectForOop: 395700481 "'aRsrConnection'"
SystemRepository listReferencesInMemory: (Array with: (Object _objectForOop: 395700481))

Since I think services should be able to be persisted - even if just inadvertently - RSR shouldn't prevent that by holding onto instances of classes that are non-persistent objects.

kurtkilpela commented 1 year ago

I believe, early on, it was decided that RsrService instances would not support commit once registered with a Session (Connection). This may be worth re-evaluating. We should consider setting options on the RsrService class to make that explicit.

kurtkilpela commented 1 year ago

If possible, we may also consider allowing a Service to be committed but preventing the committing of the instance variables declared in RsrService.

martinmcclure commented 1 year ago

My recollection is similar to Kurt's -- services are not supposed to be committed; to allow that wouldn't work very well, and would complicate the RSR implementation. Yes, setting the instancesNonPersistent option on RsrService would help catch this kind of problem earlier and make for an easier-to-understand error message.

ericwinger commented 1 year ago

I don't necessarily want to commit a service but I shouldn't be prevented from committing if a service gets stuck in a persistent object by mistake. That's an opportunity for a loss of work which is very frustrating and hard to debug.

ericwinger commented 1 year ago

I found the place in my code where some services were being put in UserGlobals. Method services retained their history beyond the life of the current session. While I can work around this need by not saving the service explicitly in UserGlobals (maybe save the class name, selector, source and meta only?) it still may be a good idea to allow services to persist. For example, if a developer is using the service as a model with persistent state as I was.

At minimum, if services are to remain non-persistable, it would be helpful if the RsrConnection instance was explicitly not persistable rather than just the Semaphore in the spigot. Easier to debug.

kurtkilpela commented 1 week ago

After talking to Eric, it seems like he would be comfortable with us marking RsrConnection as non-persistent in GemStone. This would make it easier to debug.

I don't yet know if I can mark RsrConnection as non-persistent without having to move it into a GemStone-specific package. I'd really like to avoid doing that.